[amp-experiment] Exposes isDismissed() method in AmpUserNotification #3832
Conversation
… as well as some minor code clean up.
| */ | ||
| get(id) { | ||
| this.managerReadyPromise_.then(() => { | ||
| if (this.win_.document.getElementById(id) == null) { | ||
| user.warn(TAG, `Did not find amp-user-notification element ${id}.`); | ||
| } | ||
| }); | ||
| return this.getElementDeferById_(id).promise; |
There was a problem hiding this comment.
Wondering if we had a bug here. Should this be returned inside the managerReadyPromise chain? @erwinmombay
There was a problem hiding this comment.
looking at the code again, it shouldn't be a bug. although yes a bit weird. I don't think we need to be dependent on managerReady (viewer + dom ready) to return the promise (since that promise is resolved independently). this was really just to give the publisher dev a warning if they mistyped the notification "id". although you could argue as that being a user error instead of a warning and that we could move this dom scan to the url replacement code.
|
@erwinmombay NotificationInterface is never used. Should we remove it? |
| dev.error(TAG, 'Failed to read storage', reason); | ||
| return false; | ||
| }) | ||
| .then(persistedValue => !!persistedValue); |
There was a problem hiding this comment.
We can move this into the previous promise block:
.then(storage => storage.get(this.storageKey_))
.then(value => !!value, reason => {
dev.error(TAG); //...
return false;
});
| } | ||
| return this.storagePromise_ | ||
| .then(storage => storage.get(this.storageKey_)) | ||
| .then(persistedValue => !!persistedValue, reason => { |
There was a problem hiding this comment.
any reason why we're doing the coercion here instead of the previous then success handler?
also prefer .catch for error handler.
There was a problem hiding this comment.
also prefer .catch for error handler.
I told him to combine since !!value can never throw, so no reason to create another promise in the chain to catch it and any previous rejections.
There was a problem hiding this comment.
nvm i see why now. this lgtm.
|
@lannka lets keep NotificationInterface (it is used by AmpUserNotification) for its public API |
| } | ||
|
|
||
| /** | ||
| * Register an instance of `amp-user-notification`. | ||
| * @param {string} id | ||
| * @param {!UserNotification} userNotification | ||
| * @param {!AmpUserNotification} userNotification |
There was a problem hiding this comment.
this probably should have been NotificationInterface in the first place.
|
just some typing requests and addition of the method signature to the interface. otherwise LGTM |
* master: (236 commits) trim all the columns (ampproject#3894) Refactoring: Turn private custom element methods into functions. (ampproject#3882) Lower the load priority of ad shaped iframes. (ampproject#3863) JsDoc fix (ampproject#3892) Add screenshots for Opera to AMP Validator extension. (ampproject#3866) Fix renaming of generated JSCompiler_prototypeAlias variable. (ampproject#3887) fix typo in amp-sidebar.md (ampproject#3833) Validator Roll-up (ampproject#3885) [CryptoService] Leverage browser native Crypto API to hash strings. (ampproject#3850) Size update (ampproject#3883) copy amp-ad docs to builtins (ampproject#3879) move doc to extension (ampproject#3878) [amp-experiment] Exposes isDismissed() method in AmpUserNotification (ampproject#3832) fix action-impl warning on dist (ampproject#3867) Add params for microad (ampproject#3827) Fixed some A4A tests. (ampproject#3859) Updates to colanalytics vendor config for amp-analytics. (ampproject#3849) Changes to implement A4A (AMP ads for AMP pages) (ampproject#3534) Addresses comment left over from PR#3841 (ampproject#3853) Expose submit event with on=submit:el.action syntax. (ampproject#3739) ...
Also makes the UserNotificationManager.get() to return a promise of AmpUserNotification object.
isDismissed() will later be used by amp-experiment to check if a notification has been dismissed before (consent has been accepted). If not, the experiment will not take effect on the current page view. Experiment should never wait for a consent on the current page view.
Needed by #1411